Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce cider-ignored-error-phases #3423

Merged
merged 3 commits into from
Aug 22, 2023
Merged

Introduce cider-ignored-error-phases #3423

merged 3 commits into from
Aug 22, 2023

Conversation

vemv
Copy link
Member

@vemv vemv commented Aug 21, 2023

Commits:

  • Use cider-nrepl 0.36.0
    • Supports this PR, and also broadens the semantics of the tooling filter:
image

QA

This PR's main focus is easy to QA:

  • (setq cider-show-error-buffer 'always)
  • enter an invalid token like ::::a in the REPL
    • no stacktrace will show up, a concise message will be printed at the repl
  • enter an invalid macroexpansion like (let [1]) in the REPL
    • no stacktrace will show up, a concise message will be printed at the repl
  • enter an expr that will cause a runtime exception like (/ 2 0)
    • the *cider-error* buffer will show up
    • if the tooling filter is on, notably few stack frames will be visible
    • the usual output will also be visible at the REPL output.

Cheers - V

@vemv vemv requested a review from bbatsov August 21, 2023 17:10
@vemv
Copy link
Member Author

vemv commented Aug 21, 2023

@yuhan0 : feel free to review this one, I reckon it might interest you

bbatsov
bbatsov reviewed Aug 22, 2023
View reviewed changes
cider-eval.el Outdated
@@ -478,32 +485,63 @@ op/situation that originated this error."
(let ((error-buffer (cider-new-error-buffer #'cider-stacktrace-mode error-types)))
(cider-stacktrace-render error-buffer (reverse causes) error-types))))

(defun cider--handle-stacktrace-response (response causes)
"Handle stacktrace op RESPONSE, aggregating the result into CAUSES.
(defcustom cider-ignored-error-phases '("read-source"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this name is very confusing, as it doesn't indicate any relationship with the stacktrace display functionality. I also have issue with the term "ignored", as I don't think it reflects what we are really doing here.

Copy link
Member

@bbatsov bbatsov Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps just name this cider-clojure-compilation-error-phases or something like this? I doubt anyone would be changing this one much. As noted by you:

A commonly desirable thing would be that one is presented detailed error reports on application-level exceptions. Compile-time exceptions would not deserve them, by default.

bbatsov
bbatsov reviewed Aug 22, 2023
View reviewed changes
cider-eval.el Outdated
)
"Clojure error phases which will not trigger a UI to become visible.

Those UIs include, at the moment:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given there's just one UI, I'm guessing you can simplify the wording here. :-)

@bbatsov
Copy link
Member

bbatsov commented Aug 22, 2023

You also need to update the "dealing with errors" docs.

@vemv vemv force-pushed the 3418--cider-ignored-error-phases branch from a650680 to 2d80720 Compare August 22, 2023 07:08
@vemv
Copy link
Member Author

vemv commented Aug 22, 2023

Thanks for the review!

I've amended the last commit applying all feedback.

@vemv vemv force-pushed the 3418--cider-ignored-error-phases branch from 2d80720 to 003ebfa Compare August 22, 2023 13:11
@vemv
Copy link
Member Author

vemv commented Aug 22, 2023

Amended again

The only change:

In the docs I call defcustom "configuration option, as I think it's more newcomer friendly.

@bbatsov bbatsov merged commit c8080cd into master Aug 22, 2023
26 checks passed
@bbatsov bbatsov deleted the 3418--cider-ignored-error-phases branch August 22, 2023 13:32
@bbatsov
Copy link
Member

bbatsov commented Aug 22, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbatsov bbatsov bbatsov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cider-show-error-buffer/cider-auto-jump-to-error: honor :clojure.error/phase
2 participants
@vemv @bbatsov